Skip to content

chore: update to firebase 8x #940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Dec 7, 2020
Merged

chore: update to firebase 8x #940

merged 8 commits into from
Dec 7, 2020

Conversation

aperron
Copy link

@aperron aperron commented Nov 9, 2020

No description provided.

@aperron aperron closed this Nov 9, 2020
@aperron aperron reopened this Nov 9, 2020
@0xcybertim
Copy link

Any idea when/if this will be merged? I just updated the Nuxt Firebase module which has firebase v8 as a requirement.

@codecov-io
Copy link

codecov-io commented Nov 27, 2020

Codecov Report

Merging #940 (b8732f7) into master (d5f5ec2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #940   +/-   ##
=======================================
  Coverage   94.89%   94.89%           
=======================================
  Files          14       14           
  Lines         372      372           
  Branches       64       64           
=======================================
  Hits          353      353           
  Misses          1        1           
  Partials       18       18           
Impacted Files Coverage Δ
...ackages/@posva/vuefire-core/src/firestore/index.ts 100.00% <ø> (ø)
packages/@posva/vuefire-core/src/rtdb/index.ts 100.00% <ø> (ø)
packages/vuefire/src/firestore.ts 89.74% <ø> (ø)
packages/vuefire/src/rtdb.ts 91.11% <ø> (ø)
packages/vuexfire/src/firestore.ts 90.00% <ø> (ø)
packages/vuexfire/src/rtdb.ts 86.66% <ø> (ø)
...ackages/@posva/vuefire-core/src/firestore/utils.ts 100.00% <100.00%> (ø)
packages/@posva/vuefire-core/src/rtdb/utils.ts 90.00% <100.00%> (ø)
packages/@posva/vuefire-core/src/shared.ts 85.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5f5ec2...ac4a707. Read the comment docs.

@aperron
Copy link
Author

aperron commented Nov 27, 2020

Hello,
to simplify review of this pull request.

summary content:

  • dependency firebase in package.json ^7.2.3 -> ^8.1.1
  • import { XXX } from 'firebase' changed to import firebase from 'firebase/app'.
  • versions (i'm not sure, if it's necessary):
    • @posva/vuefire-core version "2.3.3" changed to "2.3.4"
    • vuefire version "2.2.4" changed to "2.2.5"
    • vuexfire version "3.2.4" changed to "3.2.5"

that all.

minor:

  • in firebase 8.0.0. timestampsInSnapshots has been remove from FirestoreSettings. So documentation could be clean up. see firebase release note

@aperron
Copy link
Author

aperron commented Nov 28, 2020

For this PR, i trusted the tests to validate that it has no regression.
But i wanted to test in my personal project. And the result is not good.


My error:
I use bindFirestoreRef(path, ref_to_collection). This collection contains objects that have a Firebase document reference on a field. The function isDocumentRef don't detect this child like a Firebase DocumentReference (no onSnapshot method on child object)

I tried a quick fix:

function isDocumentRef(o) {
    // return (o && o.onSnapshot)
    return (o && o.firestore)
}
// ...
function recursiveExtract(...){
   // ...
   if (isDocumentRef(ref)) {
       ref = ref.firestore.app.firestore().doc(ref.path)

This looks ok, but I don't see any changes in the Firebase release note that might explain this.


I'm not sure how to install it locally.

  1. yarn build on the root folder of vuefire git clone
  2. on my project:
    • add yarn add @posva/vuefire-core@file://${GIT_CLONE_PATH}/vuefire/packages/@posva/vuefire-core/
    • add yarn add vuexfire@file://${GIT_CLONE_PATH}/vuefire/packages/vuexfire

@postva: this installation sounds good to you or you see some side effects which may explain this error ?

@glemmatim: Could you test on your side ?

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Can you remove the version changes? That will be done separately.

People recently reported some bugs with Firebase 8 but there doesn't seem to be any change in the code so I think something that is mocked from Firebase has changed in v8. I'm concerned this could be a breaking change too and might need to release in a v3 of vuefire

Edit: as you mentioned, it's the onSnapshot function not existing anymore. Will have to do more research

@posva
Copy link
Member

posva commented Nov 30, 2020

Created firebase/firebase-js-sdk#4134 to know more about it since we don' have access to firestore inside of that function to create the reference and this looks like a breaking change in firebase

@posva
Copy link
Member

posva commented Nov 30, 2020

Apparently it's being tracked at firebase/firebase-js-sdk#4125

@aperron
Copy link
Author

aperron commented Dec 4, 2020

My last test with [email protected] is OK. The bug without onSnapshot method has been fixed.

I also deleted the changes in the versions.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@posva posva changed the title update firebase 7.24.0 -> 8.0.1 chore: update to firebase 8x Dec 7, 2020
@posva posva merged commit 160db5e into vuejs:master Dec 7, 2020
@mfissehaye
Copy link

How can I upgrade my vuefire package to the latest that uses firebase v8

@charles-allen
Copy link
Contributor

charles-allen commented Dec 8, 2020

How can I upgrade my vuefire package to the latest that uses firebase v8

Change the version of VueFire in package.json to 2.3.4
Edit: sorry => 2.2.5

@mfissehaye
Copy link

2.3.4 is not available. Perhaps there are new versions 3.0.0-alpha.1. I will give that a try.

@charles-allen
Copy link
Contributor

@mfissehaye - I see you now discovered 2.2.5! Sorry about the confusion.

For future reference, I recommend the following (instead of trusting me!) - it automatically bumps your package.json to the latest (non-alpha/beta) versions:

// Only once (to install globally)
npm i -g npm-check-updates

// Update package.json
ncu -u

// Install new versions from package.json
npm i

I do recommend some caution; it will update major versions (which might include breaking changes). I usually review the git diff before committing & discard anything that's giving me problems.

@mfissehaye
Copy link

mfissehaye commented Dec 16, 2020

I am still getting error Uncaught TypeError: Cannot convert undefined or null to object with versions updated.

"vuefire": "^2.2.5",
"vuexfire": "^3.2.5"

Here is the vuexfire code

    const currentUser = this.$fire.auth.currentUser
    if (currentUser) {
      const uid = currentUser.uid
      const carts = this.$fire.firestore.collection(`users/${uid}/carts`)
      return await context.bindFirestoreRef('carts', carts, { wait: true })
    }

Note: I am subscribing to a sub collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants